fix: cache ACP configs by path by matt2e · Pull Request #8893 · aaif-goose/goose

matt2e · 2026-04-29T02:44:16Z

🤖 Updated by AI agent.

This fixes ACP config races by sharing a Config handle for each config path instead of creating a new config object per request. The default Goose config directory still uses Config::global(), while custom ACP config directories use a process-wide cache keyed by the normalized config.yaml path.

Summary

Add Config::for_config_dir(...) and ConfigHandle to select either the default global config or a cached custom-path config.
Store the selected config handle on GooseAcpAgent and reuse it for session setup, provider/model resolution, config CRUD, and secret requests.
Update AcpServer creation to read Goose mode from the same path-aware config handle.
Keep custom config directories isolated while serializing concurrent writes for the same path.
Add ACP regression tests for default/global selection, custom path sharing, equivalent path normalization, custom-dir coexistence, shared writes, and concurrent upserts.

Tests

Not run in this session (PR metadata update only).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36d4ff286a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

kalvinnchau · 2026-04-29T17:20:31Z

AI comment:

This removes the agent-owned config path, so GooseAcpAgent::new(..., config_dir, ...) no longer guarantees config reads/writes use that directory. That already affects the ACP test fixture, and it would
affect any caller that wants an ACP agent with a custom config path.

If ACP should always use process-global config, should we remove config_dir from GooseAcpAgent::new or initialize/validate the global config there? Otherwise this constructor still looks like it accepts
a custom config dir even though config access now depends on prior global initialization.

Replace all Config::new(config_dir.join(...), "goose") calls in the SACP server with Config::global(), which uses the same default path but goes through the shared singleton mutex. This: - Serializes all config read-modify-write operations (same protection as Goose 1's REST server) - Fixes a keyring bug where Config::new() hardcoded SecretStorage::Keyring and ignored the GOOSE_DISABLE_KEYRING env var - Eliminates inconsistency where some handlers already used Config::global() while config CRUD handlers used the per-instance Config::new() path - Removes the now-unnecessary config_dir field, load_config/config methods, and CONFIG_YAML_NAME import from the SACP server Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…CP server Remove the unnecessary bare block around Config::global() usage in resolve_provider_and_model_with_inventory_refresh. Since Config::global() returns &'static Config and cannot fail, the extra block scope (left over from the old fallible Config::new() error-handling pattern) serves no purpose. Removing it flattens indentation by one level. Also fix three clippy::needless_borrow warnings where &config was passed but config is already &'static Config, so the extra reference is immediately dereferenced by the compiler. Signed-off-by: Matt Toohey <contact@matttoohey.com>

Add Config::init_global(config_dir) that initializes the global singleton with a caller-provided config directory instead of the default. This addresses PR feedback that Config::global() silently ignored the config_dir field from AcpServerFactoryConfig. The server factory now calls init_global(config_dir) so that if a non-default config directory is ever passed, the global Config will use the correct path while still providing the singleton mutex serialization benefit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Build Config::init_global from the same config path stack as Config::default and return an error when an existing global config points at a different writable path. Validate that invariant from GooseAcpAgent::new so direct ACP callers cannot silently use a stale global config. Update ACP fixtures to use one process-global test config directory and add a regression test for mismatched config directories. Signed-off-by: Matt Toohey <contact@matttoohey.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66b8f26323

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Add Config::for_config_dir with a path-normalized cache so custom ACP config directories share one Config mutex and secrets cache without forcing them through the global singleton. Keep the default config path on Config::global(). Store the selected config handle on GooseAcpAgent and use it for the ACP paths that previously built a per-request Config, including session provider/model resolution and generic config/secret custom requests. Update ACP fixtures and regression coverage for default/global handling, custom-directory coexistence, equivalent path normalization, shared state, and concurrent config upserts. Signed-off-by: Matt Toohey <contact@matttoohey.com>

kalvinnchau

lgtm!

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36d2e72982

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-05-01T04:16:53Z

+        let config = Arc::new(Self::new(config_path, KEYRING_SERVICE)?);
+        cache.insert(cache_key, Arc::clone(&config));


Recreate cached Config when keyring mode changes

Config::for_config_dir now caches a single Config instance per path, but Config::new decides SecretStorage once from GOOSE_DISABLE_KEYRING/config at construction time. After an ACP client updates GOOSE_DISABLE_KEYRING via _goose/config/upsert, later secret RPCs keep using the old backend (keyring vs secrets.yaml) until restart, whereas the previous per-call Config::new(...) path re-evaluated this setting each request. This can make secret reads/writes fail or go to the wrong store immediately after a runtime toggle.

Useful? React with 👍 / 👎.

chatgpt-codex-connector Bot reviewed Apr 29, 2026

View reviewed changes

Comment thread crates/goose/src/acp/server_factory.rs Outdated

Comment thread crates/goose/src/acp/server.rs Outdated

This was referenced Apr 29, 2026

fix: handle acp requests concurrently #8781

Merged

refactor: update goose2 credential management behind provider-scoped ACP/core API #8887

Merged

Base automatically changed from jamadeo/ws-concurrency to main April 29, 2026 13:54

matt2e and others added 4 commits April 30, 2026 10:52

matt2e force-pushed the concurrent-sacp-config branch from 70382ad to 66b8f26 Compare April 30, 2026 06:43

chatgpt-codex-connector Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread crates/goose/src/acp/server.rs Outdated

Comment thread crates/goose/src/config/base.rs Outdated

kalvinnchau approved these changes Apr 30, 2026

View reviewed changes

matt2e marked this pull request as draft May 1, 2026 01:15

matt2e marked this pull request as ready for review May 1, 2026 04:12

matt2e changed the title ~~refactor: use Config::global() instead of per-instance config in SACP server~~ fix: cache ACP configs by path May 1, 2026

chatgpt-codex-connector Bot reviewed May 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cache ACP configs by path#8893

fix: cache ACP configs by path#8893
matt2e wants to merge 5 commits intomainfrom
concurrent-sacp-config

matt2e commented Apr 29, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

kalvinnchau commented Apr 29, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

kalvinnchau left a comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let config = Arc::new(Self::new(config_path, KEYRING_SERVICE)?);
		cache.insert(cache_key, Arc::clone(&config));

Conversation

matt2e commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

kalvinnchau commented Apr 29, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

kalvinnchau left a comment

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matt2e commented Apr 29, 2026 •

edited

Loading